Simulate EIP-1271 orders at creation by squadgazzz · Pull Request #4355 · cowprotocol/services

squadgazzz · 2026-04-21T19:00:21Z

Problem

Orderbook accepts an EIP-1271 order if the signer contract's isValidSignature says yes. That single check is enough to let through Aave flashloan-style orders where the signature passes but the order's post-hook can never settle. We catch these later, after they're in an auction, wasting solver cycles.

A symmetric class (Euler-style) is too strict on signatures but would actually settle fine. Those are rejected today. This PR doesn't change that, but the new metrics make them visible.

Fix

Wire the existing OrderSimulator (today reachable only via /debug/simulation) into order creation. At accept time we now run the signature check and a full-order simulation concurrently, then combine the results.

Per chain, a mode config picks what to do with the combined result:

disabled (default): same as today. Signature check only.
shadow: both run, disagreements land in metrics and logs, behaviour unchanged.
enforce: if signature passes but simulation fails, reject with HTTP 400 Eip1271SimulationFailed.

Infra errors (RPC, timeout, Tenderly) never reject in any mode. Signature-fail cases return the same InvalidEip1271Signature as before, regardless of the simulation result.

Old path: one check, signature only. New path: signature plus simulation, with a per-chain knob to decide what to do when they disagree.

Rollout

Land as disabled. Flip to shadow in prod to see the matrix on real traffic. Flip to enforce once we trust the sim and the simulator refactor lands.

Changes

shared::order_validation: new Eip1271Simulating trait, Eip1271Simulator bundle (simulator + mode + timeout) threaded into OrderValidator. Validation runs signature + sim concurrently via tokio::join! with a per-call timeout, emits an eip1271_simulation_total{signature, simulation} Prometheus matrix, upgrades enforce-mode disagreements to ValidationError::SimulationFailed.
orderbook::eip1271_simulation: OrderSimulatorAdapter wrapping the existing OrderSimulator. Temporary shim until crates/simulator owns OrderSimulator.
configs: Eip1271SimulationMode enum with disabled, shadow, enforce variants.

How to test

Unit tests in order_validation cover the signature × simulation matrix, enforce-mode rejection, fail-open on infra errors, the eip1271_skip_creation_validation path, and the no-simulator-configured path. configs tests deserialize the three modes.

Runs the OrderSimulator concurrently with the cheap isValidSignature check. In shadow mode (default), logs disagreements via metrics and structured logs but returns the cheap check's result. In enforce mode, (cheap Pass, sim Fail) is upgraded to ValidationError::SimulationFailed; other combinations stay unchanged. Infra errors never reject. Covers scope from plan Tasks 3, 4 and 5: shadow-mode quadrants, enforce-mode cases, infra/skip-flag/no-sim paths.

The Shadow/Enforce distinction is a mode variant, not a property of the capability — the same simulator infrastructure is active in both modes. Keeping "shadow" only where it names a mode variant, and renaming the trait, error, mode enum, metrics, constant, config fields, and module accordingly.

Doc comments and local variable names still described the capability as 'shadow' — rewritten to reference the mode variant only where it genuinely applies (config docs, test names that exercise Shadow mode, the Shadow enum variant).

The simulator, mode, and timeout are only meaningful together. Collapsing them into a single Option<Eip1271SimConfig> field lets the call site in run.rs return None cleanly when order_simulation isn't configured, instead of passing placeholder mode/timeout values that aren't read.

… to Simulator Matches the project's convention of -ing suffix for traits (OrderValidating/OrderValidator, SignatureValidating, BalanceFetching). The former Eip1271SimConfig bundle becomes the concrete Eip1271Simulator struct, and the trait it depends on becomes Eip1271Simulating.

The helper now accepts the full simulator bundle instead of three separate (simulator, mode, timeout) args. Introduces shadow_mode_sim / enforce_mode_sim helpers to keep test call sites tight.

The constant is only used by tests; keeping it at module scope (and public) implied an API contract with the configs crate that doesn't exist. configs::orderbook::default_eip1271_sim_timeout is authoritative at runtime.

The existing EIP-1271 check is an isValidSignature call; 'cheap' was editorializing on cost rather than describing what it does. Renaming to 'signature' for the metric axis, outcome enum, and supporting names. Also collapsing sim_only_total into total by adding a 'skipped' value to the signature axis — one counter with a signature × sim matrix covers every case the two used to cover.

…ing fallback

…configs, logs The Eip1271Simulator struct keeps its -Simulator suffix (matching OrderValidator/-Validating), but everywhere sim was used as a modifier or qualifier (enum variants, error type, metric subsystem/labels, config fields, log messages, test names) it now reads as simulation.

Operators can now opt out of the simulation at order creation on a per-chain basis without giving up the /debug/simulation endpoint. The shared mode enum stays binary (Shadow/Enforce); Disabled translates to None for OrderValidator at the wiring layer. The OrderSimulator is still constructed for the debug endpoint. Also removed the redundant impls_trait compile-check test in eip1271_simulation.rs — the impl block above already enforces that at compile time.

Mock both the signature validator and the simulator with times(0) and submit an Eip712 (EOA) order. Catches a regression where the sim is accidentally wired to run for non-1271 orders.

The seven near-identical tests covering every (signature, simulation, mode) combination collapsed into one table-driven test with a single mock-driven helper. Failure messages include a label per row so any regression still pinpoints the failing cell. Also inlined the now-unused enforce_mode_simulator helper and replaced shadow_mode_simulator with a general simulator_with_mode.

gemini-code-assist

Code Review

This pull request introduces EIP-1271 order simulation into the order validation pipeline, enabling the system to verify order execution (pre-hooks, swap, and post-hooks) before acceptance. It includes new configuration for simulation modes (Shadow and Enforce) and timeouts, as well as an adapter to bridge the order simulator with the validation logic. Feedback was provided regarding the build_preview_order_for_sim function, which currently initializes metadata with default values; the settlement_contract address must be explicitly passed to ensure simulations correctly reflect the target environment.

gemini-code-assist · 2026-04-21T19:29:18Z

+fn build_preview_order_for_sim(
+    data: &OrderData,
+    interactions: &Interactions,
+    owner: Address,
+    uid: OrderUid,
+    signature: Signature,
+) -> Order {
+    Order {
+        metadata: OrderMetadata {
+            owner,
+            uid,
+            ..Default::default()
+        },
+        data: *data,
+        signature,
+        interactions: interactions.clone(),
+    }
+}


The build_preview_order_for_sim function initializes OrderMetadata with Default::default(), which leaves the settlement_contract field as Address::ZERO. This will cause simulation failures if the simulation logic or hooks rely on the correct settlement contract address (e.g., to determine the transaction recipient). Pass the settlement_contract address as an argument and set it in the metadata.

Suggested change

fn build_preview_order_for_sim(

data: &OrderData,

interactions: &Interactions,

owner: Address,

uid: OrderUid,

signature: Signature,

) -> Order {

Order {

metadata: OrderMetadata {

owner,

uid,

..Default::default()

},

data: *data,

signature,

interactions: interactions.clone(),

}

}

fn build_preview_order_for_sim(

data: &OrderData,

interactions: &Interactions,

owner: Address,

uid: OrderUid,

signature: Signature,

settlement_contract: Address,

) -> Order {

Order {

metadata: OrderMetadata {

owner,

uid,

settlement_contract,

..Default::default()

},

data: *data,

signature,

interactions: interactions.clone(),

}

}

The simulator does not read order.metadata.settlement_contract. SwapSimulator holds its own GPv2Settlement instance captured at construction (crates/simulator/src/swap_simulator.rs:67) and uses that address directly — e.g. as the balance-override holder in crates/orderbook/src/order_simulator.rs:257. Across the workspace, the only reader of that metadata field is the DB persistence path (crates/orderbook/src/database/orders.rs:175), which this preview Order never reaches. Leaving as is.

gemini-code-assist · 2026-04-21T19:29:18Z

+            let preview_order = build_preview_order_for_sim(
+                &data,
+                &app_data.interactions,
+                owner,
+                uid,
+                order.signature.clone(),
+            );


Pass the settlement_contract address to build_preview_order_for_sim to ensure the simulation metadata is correctly populated.

Suggested change

let preview_order = build_preview_order_for_sim(

&data,

&app_data.interactions,

owner,

uid,

order.signature.clone(),

);

let preview_order = build_preview_order_for_sim(

&data,

&app_data.interactions,

owner,

uid,

order.signature.clone(),

settlement_contract,

);

squadgazzz · 2026-04-21T19:32:32Z

@claude review

claude · 2026-04-21T19:32:50Z

Claude finished @squadgazzz's task in 4m 35s —— View job

Code Review

Gather context
Read changed files
Analyze and provide review feedback

Overall this is a solid PR — clean trait abstraction, concurrent signature + simulation via tokio::join!, comprehensive test matrix, and a safe fail-open design for infra errors. A few issues worth addressing before merge:

Timer measures join latency, not simulation latency

crates/shared/src/order_validation.rs:622–627

let (signature_res, simulation_res) = {
    let _timer = Eip1271SimulationMetrics::get()
        .simulation_time
        .start_timer();
    tokio::join!(signature_fut, simulation_fut)
};

The simulation_time histogram records the wall-clock time of the join!, which is max(signature_check_time, simulation_time), not the simulation time alone. If the signature check takes 3s and simulation takes 1s, simulation_time will record 3s. This makes the histogram misleading for its stated purpose.

Contrast with run_eip1271_simulation_only:221–226 where the timer correctly wraps only the simulation call:

let _timer = Eip1271SimulationMetrics::get().simulation_time.start_timer();
tokio::time::timeout(config.timeout, sim.simulate(preview_order)).await

Fix: start the timer inside simulation_fut rather than around the join!, or rename the metric to something like eip1271_check_time that accurately describes what's being measured.

Fix this →

Log level inconsistency between the two simulation paths

run_eip1271_simulation_only:247–252 (skip-validation path) logs simulation failures at tracing::info!:

SimulationOutcome::Fail { reason, tenderly_url } => tracing::info!(
    order_uid = %preview_order.metadata.uid,
    ?reason,
    ?tenderly_url,
    "eip1271 simulation (signature check skipped)",
),

But record_simulation_outcome:199–206 (normal path) logs the same sig=pass, sim=fail disagreement at tracing::warn!. Both paths are observing a simulation failure — the different log levels could cause operational confusion when querying logs.

Fix this →

Missing config test for `shadow` mode

crates/configs/src/orderbook/mod.rs has tests for Disabled (default), Enforce, and Disabled explicit — but not for Shadow. Small coverage gap:

gas-limit = "0x1000000"
eip1271-simulation-mode = "shadow"

Fix this →

Nit: `#[default]` on `Shadow` is misleading given the operational default is `Disabled`

crates/shared/src/order_validation.rs:81–83

/// Log disagreements, emit metrics. Never reject. **Default.**
#[default]
Shadow,

The Rust-level default (via Default derive) is Shadow, but the operational default via config is Disabled (no simulator). No code path calls Eip1271SimulationMode::default() — the mode is always set explicitly from config. The Default derive + "Default." doc could mislead a reader into thinking the system defaults to Shadow. Consider either removing Default from the derive list, or at least dropping the "Default." label from the doc comment.

Response body leaks raw revert reason in enforce mode

crates/orderbook/src/api/post_order.rs:191–195

ValidationError::SimulationFailed(reason) => (
    StatusCode::BAD_REQUEST,
    error("Eip1271SimulationFailed", reason),
).into_response(),

The revert reason comes directly from the EVM simulation and is sent verbatim to API clients. This is consistent with TransferSimulationFailed precedent and is useful for order debugging, so probably acceptable — just flagging it for awareness.

- Move simulation_time histogram timer inside simulation_fut so it no longer captures the max of sig-check + simulation latency. - Warn-level (not info) for simulation failures in the eip1271_skip_creation_validation path, matching the normal path. - Drop Default derive on the shared Eip1271SimulationMode since the operational default lives in configs (Disabled) and no code reaches for it. Updated the doc to clarify the split. - New configs test asserting "shadow" deserializes to the Shadow variant.

jmg-duarte · 2026-04-22T08:11:56Z

+/// Mode for the EIP-1271 order simulation at order creation.
+#[derive(Copy, Clone, Debug, Default, Deserialize, Serialize, Eq, PartialEq)]
+#[serde(rename_all = "kebab-case")]
+pub enum Eip1271SimulationMode {
+    Shadow,
+    Enforce,
+    #[default]
+    Disabled,
+}


Shouldn't this be part of either the configs crate or the order simulator?

It is in configs, this is the enum you're commenting on. The copy in shared::order_validation is only there because OrderValidator lives in shared and shared can't depend on configs, so we mirror plus convert at the call site in run.rs. Once Martin's simulator refactor lands and OrderSimulator moves out of orderbook, we can probably collapse both into one enum.

jmg-duarte · 2026-04-22T08:29:22Z

+        }
+    }
+
+    fn build_1271_validator(


given you're adding expectations to the mocks, documenting them here with the "why" would be helpful

Added comments.

jmg-duarte · 2026-04-22T08:31:04Z

+            let label = format!("sig={sig:?} sim={simulation:?} mode={mode:?}");
+            let mut signature_validator = MockSignatureValidating::new();
+            signature_validator
+                .expect_validate_signature_and_get_additional_gas()
+                .returning(move |_| match sig {
+                    Sig::Pass => Ok(0u64),
+                    Sig::Invalid => Err(SignatureValidationError::Invalid),
+                });
+            let mut sim = MockEip1271Simulating::new();
+            sim.expect_simulate().returning(move |_| match simulation {
+                Sim::Pass => Ok(()),
+                Sim::Reverted => Err(Eip1271SimulationError::Reverted {
+                    reason: "hook reverted".into(),
+                    tenderly_url: None,
+                }),
+            });
+            let validator = build_1271_validator(
+                signature_validator,
+                Some(simulator_with_mode(sim, mode)),
+                false,
+            );
+            let result = validator
+                .validate_and_construct_order(
+                    make_1271_order_creation(),
+                    &DomainSeparator::default(),
+                    Default::default(),
+                    None,
+                )
+                .await;
+            match expected {
+                Expected::Accepted => assert!(result.is_ok(), "{label}: got {result:?}"),
+                Expected::InvalidSignature => assert!(
+                    matches!(result, Err(ValidationError::InvalidEip1271Signature(_))),
+                    "{label}: got {result:?}"
+                ),
+                Expected::SimulationFailed => assert!(
+                    matches!(result, Err(ValidationError::SimulationFailed(_))),
+                    "{label}: got {result:?}"
+                ),
+            }
+        }


Separating the test from the matrix would make sense to me + trying to add info on which combination failed and where to ease debugging

The matrix is the result of collapsing 7 nearly identical tests. Every assert! message includes sig={sig:?} sim={simulation:?} mode={mode:?}, so a failure tells you exactly which cell blew up.

jmg-duarte · 2026-04-22T08:32:24Z

+        // Build an EOA (Eip712) order and a simulator configured in enforce mode.
+        // The simulator should NOT be called — only EIP-1271 orders go through
+        // the simulation path.


the other mocks in this file could also get a note like this

Added comments.

…ture_check Same separation now used in run_eip1271_simulation_only. The simulation future returns the raw timeout Result; the outer match splits timeout (Infra) from the inner classification.

# Description The Claude code-review action currently posts one big summary as an issue comment (example: cowprotocol#4355 (comment)). The `code-review` plugin has a `--comment` flag that switches it from the summary-text mode to posting one PR review comment per issue anchored to the exact line in the diff, matching how `gemini-code-assist[bot]` already behaves. Per the plugin spec (step 9), this flag triggers `mcp__github_inline_comment__create_inline_comment` instead of a single `gh pr comment`. # Changes - Append `--comment` to the `/code-review:code-review` prompt in `.github/workflows/claude-code-review.yml`. # How to test Run the `claude /review` command.

jmg-duarte

No blockers from me, just small nits/improvements

- today's -> current in run_eip1271_with_signature_check docstring - Add timed_simulation helper folding metrics, timeout, and outcome classification. Replaces the duplicated wrapper at both call sites. - Extract calculate_verification_gas_limit so the EIP-1271 setup no longer lives inline in validate_and_construct_order.

fleupold · 2026-04-27T19:05:15Z

+#[async_trait]
+impl Eip1271Simulating for OrderSimulatorAdapter {
+    async fn simulate(&self, order: &Order) -> Result<(), Eip1271SimulationError> {
+        let swap = self.inner.encode_order(order, Vec::new(), None).await?;


How does this logic work for Aave orders that require a flashloan and are routed via the FlashloanWrapper contract instead of directly via GPv2Settlement?

If we don't pass in any wrappers here, how will it encode the transaction in a way that simulation can pass?

Is there a way we can test this case in our existing Aave integration test?

Other simulations side stepped the flashloan aspect by using state overrides to make the sell_tokens appear. But I also can't find this logic in the current PR.
The overhaul of the simulation crate I'm working on would add proper support for flashloans as well.

fleupold · 2026-04-27T19:12:48Z

+    Infra(anyhow::Error),
+}
+
+/// Simulates an EIP-1271 order end-to-end (pre-hooks → swap → post-hooks).


This is not really correct. We don't actually simulate the full swap logic, in particular the isValidSignature is replaced by Trader.sol which always returns the magic value (true).

I think it would actually be nice if we could use only one method to verify everything end-to-end, instead of having one code path for signature verification and one for pre/post hooks, but I'm not sure what's all required for this.
At least for the signature check, we'd need to delegatecall into the original smart contract bytecode (for EIP1271 orders).

The current simulator logic seems to have preferred building something that usable for the quote verification over the "normal" simulation case where we basically have to do no trickery. This makes the logic here quite awkward. I'm currently working on a simulation builder that makes it flexible enough to work with the quote verification and the normal use case to make it clear what's needed for what.

Ok, we should at least address this comment then to not overpromise what this trait does.

squadgazzz · 2026-04-30T14:06:22Z

in favor of #4366

squadgazzz added 29 commits April 21, 2026 17:30

Add EIP-1271 shadow simulator scaffolding

6897fec

Fix clippy and align SimulationFailed with tuple convention

4d63688

Thread optional EIP-1271 shadow simulator into OrderValidator

7eb008b

Extract shadow sim timeout constant and rename HTTP error code

3d074e4

Add shadow sim mode and timeout to orderbook config

f0af347

Wire EIP-1271 shadow simulator through orderbook runtime

bd51ed8

Nits

6e7ee7a

Collapse build_1271_validator args to take Option<Eip1271Simulator>

65a0290

The helper now accepts the full simulator bundle instead of three separate (simulator, mode, timeout) args. Introduces shadow_mode_sim / enforce_mode_sim helpers to keep test call sites tight.

Move DEFAULT_EIP1271_SIM_TIMEOUT into test module

a37c7a0

The constant is only used by tests; keeping it at module scope (and public) implied an API contract with the configs crate that doesn't exist. configs::orderbook::default_eip1271_sim_timeout is authoritative at runtime.

Simplify Eip1271Simulator doc comment

9c93de6

Rename duration_seconds histogram to simulation_time

cebd99b

Drop owner from sim logs; OrderUid already encodes it

67a4d32

Split sim disagreement log into explicit arms instead of an empty-str…

2fbf9f0

…ing fallback

Use Debug formatting (?) in sim logs to match project convention

a781c44

Simplify Eip1271Simulating doc and scope its mock to cfg(test) only

eca5c61

Disabled by default

eed5990

Align default test with Disabled as the default simulation mode

2785108

Redundant comment

7281ded

Assert simulator is never invoked for non-EIP-1271 orders

3525684

Mock both the signature validator and the simulator with times(0) and submit an Eip712 (EOA) order. Catches a regression where the sim is accidentally wired to run for non-1271 orders.

Use .label() on outcomes instead of hardcoded strings in sim logs

e62217c

squadgazzz marked this pull request as ready for review April 21, 2026 19:25

squadgazzz requested a review from a team as a code owner April 21, 2026 19:25

squadgazzz changed the title ~~Shadow-simulate EIP-1271 orders at creation~~ Simulate EIP-1271 orders at creation Apr 21, 2026

gemini-code-assist Bot reviewed Apr 21, 2026

View reviewed changes

squadgazzz mentioned this pull request Apr 21, 2026

Use inline PR review comments from Claude code review #4356

Merged

fafk reviewed Apr 22, 2026

View reviewed changes

Comment thread crates/orderbook/src/eip1271_simulation.rs Outdated

Convert order_simulator::Error via From impl for ? ergonomics

250f5da

jmg-duarte reviewed Apr 22, 2026

View reviewed changes

squadgazzz added 4 commits April 22, 2026 10:04

Review comments

c51a522

Nit

7bd2592

Add SignatureCheck::new constructor

ea11e37

Pull timeout handling out of simulation_fut in run_eip1271_with_signa…

ebfd4a9

…ture_check Same separation now used in run_eip1271_simulation_only. The simulation future returns the raw timeout Result; the outer match splits timeout (Infra) from the inner classification.

squadgazzz requested a review from jmg-duarte April 22, 2026 10:12

Merge branch 'main' into feat/orderbook-eip1271-shadow-sim

06c478a

jmg-duarte approved these changes Apr 24, 2026

View reviewed changes

Comment thread crates/shared/src/order_validation.rs Outdated

Comment thread crates/shared/src/order_validation.rs

Comment thread crates/shared/src/order_validation.rs Outdated

Comment thread crates/shared/src/order_validation.rs Outdated

squadgazzz requested a review from jmg-duarte April 27, 2026 09:57

jmg-duarte approved these changes Apr 27, 2026

View reviewed changes

fleupold reviewed Apr 27, 2026

View reviewed changes

Comment

fbe765c

squadgazzz marked this pull request as draft April 29, 2026 08:33

squadgazzz closed this Apr 30, 2026

github-actions Bot locked and limited conversation to collaborators Apr 30, 2026

Conversation

squadgazzz commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Rollout

Changes

How to test

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

squadgazzz Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squadgazzz commented Apr 21, 2026

Uh oh!

claude Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Timer measures join latency, not simulation latency

Log level inconsistency between the two simulation paths

Missing config test for shadow mode

Nit: #[default] on Shadow is misleading given the operational default is Disabled

Response body leaks raw revert reason in enforce mode

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squadgazzz commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

squadgazzz commented Apr 21, 2026 •

edited

Loading

squadgazzz Apr 21, 2026 •

edited

Loading

claude Bot commented Apr 21, 2026 •

edited

Loading

Missing config test for `shadow` mode

Nit: `#[default]` on `Shadow` is misleading given the operational default is `Disabled`